feat(appender-tracing): propagate span name to logs#3466
feat(appender-tracing): propagate span name to logs#3466SuperFluffy wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
changelog
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3466 +/- ##
======================================
Coverage 84.1% 84.2%
======================================
Files 126 126
Lines 26720 26841 +121
======================================
+ Hits 22490 22611 +121
Misses 4230 4230 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Agree we need to provide a way to attach span name, but need to also avoid picking an attribute name ourselves. See
#3466 (comment)
| /// (the default) disables it. | ||
| #[cfg(feature = "experimental_span_attributes")] | ||
| pub fn with_span_name(mut self, key: impl Into<Key>) -> Self { | ||
| self.span_name_key = Some(key.into()); |
There was a problem hiding this comment.
Note sure in how far this matters, but key here should be set once and be alive until the end of the program. Does it make sense to Box::leak::<'static> the key/string?
There was a problem hiding this comment.
given this is tracing related, and tracing only has &static` keys, lets also accept that here?
There was a problem hiding this comment.
given this is tracing related, and tracing only has
&statickeys, lets also accept that here?
Can you clarify? Do you mean we should only have
pub fn with_span_name(mut self, key: &'static str) -> Self {There was a problem hiding this comment.
yes. We can internally make a Key cheaply using the static str.
There was a problem hiding this comment.
Done. It's using static string refs now. Figured you don't want to Box::leak any arguments to make them 'static.
Did the changes. I guess this leaves open the question what to do if span and attribute names clash - but I don't think this crate should handle. Last time I checked the tracing crate itself just uses the last value it finds for a given key, so this is consistent. |
The clash is not handled by the OTel api/sdk, so it is left to users. (Something to be revisited, but definitely outside scope of this PR) |
|
Updated the PR and removed the feature flags since span attribute propagation was stabilized. |
| /// attribute key. Setting this enables the propagation; leaving it unset | ||
| /// (the default) disables it. | ||
| /// | ||
| /// This does nothing if `Self::with_tracing_span_attributes` is not set. |
There was a problem hiding this comment.
The interaction with TracingSpanAttributes is odd now.
Should we scrap with_span_name in favor of extending TracingSpanAttributes (kinda like OpenOptions)?
There was a problem hiding this comment.
@SuperFluffy This requires some discussion to make sure we do a holistic approach. Given we are doing a release today, I reverted and kept the attribute enrichment gated behind feature flag (#3505).
I'll proceed with the release today, and after that, will get back to this. (Sorry we'll miss span-name enrichment in this release, but we can do ad-hoc release of this crate soon, so we don't have to wait for next full release).
2 participants
Changes
Propagates the tracing span name to OTEL logs.
Similar to the experimental span attributes, the span containing the event is valuable to identify where and what happened.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes